Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(VirtualizedList)!: remove numberOfElementsVisibleOnScreen and numberOfElementsRendered props #153

Merged
merged 12 commits into from
Oct 8, 2024

Conversation

JulienIzz
Copy link
Contributor

@JulienIzz JulienIzz commented Sep 17, 2024

Previously, the SpatialNavigationVirtualizedList component had a prop called numberOfItemsVisibleOnScreen which has been renamed to additionalItemsRendered. This simplifies the component API and provides the end user an easy way to setup a virtualized list with safe rendering and virtualization.
Same change was made to 'SpatialNavigationVirtualizedGrid`.

Summary :

  • Delete numberOfItemsVisibleOnScreen prop => Now computed automatically
  • Delete numberOfItemsRendered prop
  • Add additionalItemsRendered with safe default values
  • Fix types error
  • Add a build task to test types
  • Update docs

Switching to the new version

In the latest version, the numberOfItemsVisibleOnScreen and numberOfItemsRendered props have been removed from the virtualized lists and grid API. The numberOfItemsVisibleOnScreen is now calculated automatically based on the parent view size.

Both of these props have been replaced by a new optional prop called additionalItemsRendered. This prop controls the number of items rendered just outside the visible screen (but not yet virtualized). It allows you to fine-tune the virtualization size without the need to manually calculate numberOfItemsVisibleOnScreen, which could lead to incorrect values.

How to Update to the New Version

To migrate to the new version, you'll need to remove the numberOfItemsVisibleOnScreen and numberOfItemsRendered props from all instances of SpatialNavigationVirtualizedList and SpatialNavigationVirtualizedGrid.

By default, you don't need to provide a value for additionalItemsRendered. However, if you want to replicate the behavior of the previous version exactly, you can set additionalItemsRendered to numberOfItemsRendered - numberOfItemsVisibleOnScreen. This will ensure the same number of off-screen items are rendered.

Impact on Tests

Your tests may also break, as the number of visible items is now calculated based on the size of the parent view. This number will vary depending on the execution environment. For example, in Jest, where the window width is 750 px, the calculation will be based on that width.

@JulienIzz JulienIzz force-pushed the feat/remove-computable-props branch from 2f518fb to 80276ad Compare September 17, 2024 09:58
@JulienIzz JulienIzz force-pushed the feat/remove-computable-props branch from 80276ad to 5ffeda3 Compare September 18, 2024 08:45
return minSize;
};
export const getNumberOfItemsVisibleOnScreen = <T>(
data: T[],
Copy link

@MathieuFedrigo MathieuFedrigo Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

déso je débarque un peu car j'ai quasiment jamais relu de PR sur la lib 😬

Mais data c'est pas très clair. C'est des items ? Autant que le naming soit cohérent vu qu'on a itemSize juste derriere.

Le type générique est pas clair non plus: T ça veut rien dire -> c'est ItemType ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(je réponds en anglais comme c'est public)

We used "data" to be as close as possible to the FlatList API. That allows us in the future to have an easier time when working with universal apps :) (FlatList on mobile, SpatialNavigationVirtualizedList on TV, and props will be similar)

Copy link

@MathieuFedrigo MathieuFedrigo Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against using data as the API prop. But I don't think we should use it internally. We can always add an adaptation layer, or at least use item everywhere else than inside the exported component

I'm also unresolving the comment because I don't think the type should be called T 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this is not resolved!

I totally agree with T that could be named TData for consistency. But I think I disagree with data renaming: it's quite obvious that we're talking about the data props, untransformed, that's given to the list. I think renaming it internally would create add more confusion than clarity 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still thinking about it: I think it really makes sense to rename data at some point internally, but we need to think the name through and do it consistently everywhere 😁
It could be the purpose of a refactoring PR!

@JulienIzz JulienIzz force-pushed the feat/remove-computable-props branch from 1abc64a to dab9e29 Compare September 18, 2024 10:00
@JulienIzz JulienIzz force-pushed the feat/remove-computable-props branch from a50198f to 28235ed Compare September 20, 2024 08:52
@pierpo pierpo merged commit 2268059 into main Oct 8, 2024
1 check passed
@pierpo pierpo deleted the feat/remove-computable-props branch October 8, 2024 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants